-
-
Notifications
You must be signed in to change notification settings - Fork 693
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ignoringFields with different map size #3003
base: 3.x
Are you sure you want to change the base?
Fix ignoringFields with different map size #3003
Conversation
Fix ignoringFields behavior that doesn't work properly when using maps of different sizes with usingRecursiveComparison and isEqualTo for example. Fix assertj#2988
@@ -557,6 +559,15 @@ private static void compareUnorderedMap(DualValue dualValue, ComparisonState com | |||
} | |||
} | |||
|
|||
private static Map<?, ?> filterIgnoringFields(Map<?, ?> dualValue, ComparisonState comparisonState) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's rename dualValue
to map
since this is not a dualvalue
return dualValue; | ||
} | ||
return dualValue.entrySet() | ||
.stream().filter(e -> !ignoredFields.contains(e.getKey())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename e
to entry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are comparing the map key interpreted as field against the ignored fields, this is not entirely correct because we need to compare the field location (FieldLocation
) to the ignored field.
Let's say we want to ignore the person.friends.address
field, the code would only compare address
so if it turns out that the root object as an address
field then this will be ignored when it should not.
Moreover the recursive comparison supports ignoring fields by regex.
Have a look at matchesAnIgnoredFieldRegex
and matchesAnIgnoredField
in AbstractRecursiveOperationConfiguration
We should also add test cases in where we ignore for maps:
- fields by regex
- nested fields
thanks @OlivierCavadenti, first round of review done! |
38c7b4b
to
3cde5bf
Compare
@OlivierCavadenti have you got time to finish this PR, if not, that's ok I can finish (you'll get full credit for it anyway 😉) |
I'm taking over this one @OlivierCavadenti as the fix is more complex than I thought, but you'll get credit 😉 . The fix should check if the key is to be ignored according to the configuration, that includes:
It is likely that we would need to compute the differences as if fields were not ignored but discard them when registering the diff as we do with the regular fields |
Hi ! |
301ca01
to
c730d18
Compare
Fix ignoringFields behavior that doesn't work properly when using maps of different sizes with usingRecursiveComparison and isEqualTo for example.
Fix #2988